Support configurable and auto-detected output format for cropped images#34
Support configurable and auto-detected output format for cropped images#34paodb wants to merge 2 commits into
Conversation
WalkthroughAdds configurable cropped-image output MIME type and quality, with client-side format detection, circular-crop PNG fallback, a new demo, unit tests, README updates, and a version bump. ChangesOutput format configuration
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/test/java/com/flowingcode/vaadin/addons/imagecrop/test/ImageCropTest.java (1)
48-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding tests for default (auto-detect) state.
These tests only verify explicit set/get round-trips. The PR's headline feature is auto-detection when
outputMimeType/outputQualityare left unset (null). Consider adding a test assertinggetOutputMimeType()returnsnullby default (before anysetOutputMimeTypecall) to lock in the auto-detect contract.✅ Suggested additional test
+ `@Test` + public void testGetOutputMimeTypeDefaultsToNull() { + assertNull(imageCrop.getOutputMimeType()); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/flowingcode/vaadin/addons/imagecrop/test/ImageCropTest.java` around lines 48 - 61, Add a test in ImageCropTest that exercises the default auto-detect state before any setters are called: verify imageCrop.getOutputMimeType() returns null initially, and if applicable do the same for outputQuality. Use the existing ImageCrop test fixture and the getOutputMimeType()/getOutputQuality() accessors to lock in the unset/null contract, instead of only testing set/get round-trips.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/flowingcode/vaadin/addons/imagecrop/ImageCrop.java`:
- Around line 143-154: The refreshCroppedImage() flow currently depends on
setState(...) having already been applied before
getElement().executeJs("_updateCroppedImage(...)") runs, which is not guaranteed
by Vaadin. Update the ImageCrop logic so the re-encode uses client-side state
consistently, either by moving the update into a client-side effect or by
passing the new crop/mime/quality values directly from the caller into
refreshCroppedImage()/_updateCroppedImage(). Ensure the fix is applied where
property-change handlers trigger refreshCroppedImage() so the client always uses
the intended latest values.
- Around line 378-398: `ImageCrop#getOutputQuality()` can still fail if the
server-side state has never been initialized, even though the client defaults to
1.0. Update `ImageCrop` to initialize `outputQuality` on the Java side
(preferably in the constructor) or make `getOutputQuality()` fall back to 1.0
when the state is absent, and keep the existing `setOutputQuality` /
`refreshCroppedImage` flow unchanged.
In `@src/main/resources/META-INF/resources/frontend/src/image-crop.tsx`:
- Around line 300-302: The server-side output quality path can unbox a null
primitive before the client initializes its fallback value, so mirror the
client’s 1.0 default there. Update the relevant server-side getter or
initializer for output quality so it safely falls back to 1.0 when the nullable
state is unset, keeping behavior aligned with the frontend’s `this.outputQuality
?? 1.0` in `image-crop.tsx`.
---
Nitpick comments:
In
`@src/test/java/com/flowingcode/vaadin/addons/imagecrop/test/ImageCropTest.java`:
- Around line 48-61: Add a test in ImageCropTest that exercises the default
auto-detect state before any setters are called: verify
imageCrop.getOutputMimeType() returns null initially, and if applicable do the
same for outputQuality. Use the existing ImageCrop test fixture and the
getOutputMimeType()/getOutputQuality() accessors to lock in the unset/null
contract, instead of only testing set/get round-trips.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 50c61048-98f9-49a4-bbb3-29d5cccd1bbb
📒 Files selected for processing (7)
README.mdpom.xmlsrc/main/java/com/flowingcode/vaadin/addons/imagecrop/ImageCrop.javasrc/main/resources/META-INF/resources/frontend/src/image-crop.tsxsrc/test/java/com/flowingcode/vaadin/addons/imagecrop/ImageCropDemoView.javasrc/test/java/com/flowingcode/vaadin/addons/imagecrop/OutputFormatImageCropDemo.javasrc/test/java/com/flowingcode/vaadin/addons/imagecrop/test/ImageCropTest.java
63bd5fa to
3545968
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/com/flowingcode/vaadin/addons/imagecrop/ImageCrop.java (1)
387-389: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider validating
outputQualitybounds.
setOutputQualityaccepts anydoublewith no range check, though the Javadoc states "a value between 0 and 1". Out-of-range values are silently passed through tocanvas.toDataURLon the client.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/flowingcode/vaadin/addons/imagecrop/ImageCrop.java` around lines 387 - 389, `ImageCrop.setOutputQuality` currently passes any double through without enforcing the documented 0-to-1 range. Add validation in this setter to reject or clamp out-of-bounds values before calling `setState("outputQuality", ...)`, so the method matches its Javadoc and only sends valid quality values to the client.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/main/java/com/flowingcode/vaadin/addons/imagecrop/ImageCrop.java`:
- Around line 387-389: `ImageCrop.setOutputQuality` currently passes any double
through without enforcing the documented 0-to-1 range. Add validation in this
setter to reject or clamp out-of-bounds values before calling
`setState("outputQuality", ...)`, so the method matches its Javadoc and only
sends valid quality values to the client.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6eb90f1-5355-405a-b6a3-23064cb9a113
📒 Files selected for processing (7)
README.mdpom.xmlsrc/main/java/com/flowingcode/vaadin/addons/imagecrop/ImageCrop.javasrc/main/resources/META-INF/resources/frontend/src/image-crop.tsxsrc/test/java/com/flowingcode/vaadin/addons/imagecrop/ImageCropDemoView.javasrc/test/java/com/flowingcode/vaadin/addons/imagecrop/OutputFormatImageCropDemo.javasrc/test/java/com/flowingcode/vaadin/addons/imagecrop/test/ImageCropTest.java
✅ Files skipped from review due to trivial changes (2)
- pom.xml
- README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/java/com/flowingcode/vaadin/addons/imagecrop/ImageCropDemoView.java
- src/test/java/com/flowingcode/vaadin/addons/imagecrop/OutputFormatImageCropDemo.java
- src/main/resources/META-INF/resources/frontend/src/image-crop.tsx
3545968 to
9f06011
Compare
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/com/flowingcode/vaadin/addons/imagecrop/ImageCrop.java (1)
340-392: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract property-name constants to fix SonarCloud duplication failures.
"outputMimeType"and"outputQuality"are each repeated 3 times (setter,getPropertyRawcheck,getStatecall). SonarCloud flags both as quality-gate failures.♻️ Proposed fix
+ private static final String OUTPUT_MIME_TYPE_PROPERTY = "outputMimeType"; + private static final String OUTPUT_QUALITY_PROPERTY = "outputQuality"; + public void setOutputMimeType(String outputMimeType) { - setState("outputMimeType", outputMimeType); + setState(OUTPUT_MIME_TYPE_PROPERTY, outputMimeType); } public String getOutputMimeType() { - if (getElement().getPropertyRaw("outputMimeType") == null) { + if (getElement().getPropertyRaw(OUTPUT_MIME_TYPE_PROPERTY) == null) { return null; } - return getState("outputMimeType", String.class); + return getState(OUTPUT_MIME_TYPE_PROPERTY, String.class); } public void setOutputQuality(double outputQuality) { - setState("outputQuality", outputQuality); + setState(OUTPUT_QUALITY_PROPERTY, outputQuality); } public double getOutputQuality() { - if (getElement().getPropertyRaw("outputQuality") == null) { + if (getElement().getPropertyRaw(OUTPUT_QUALITY_PROPERTY) == null) { return 1.0; } - return getState("outputQuality", Double.class); + return getState(OUTPUT_QUALITY_PROPERTY, Double.class); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/flowingcode/vaadin/addons/imagecrop/ImageCrop.java` around lines 340 - 392, The ImageCrop output property names are duplicated in multiple places, triggering SonarCloud duplication warnings. Extract shared constants for the state keys used by setOutputMimeType/getOutputMimeType and setOutputQuality/getOutputQuality, and reuse those constants in the setState, getPropertyRaw, and getState calls. Keep the existing behavior unchanged while centralizing the literal names in ImageCrop.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/main/java/com/flowingcode/vaadin/addons/imagecrop/ImageCrop.java`:
- Around line 340-392: The ImageCrop output property names are duplicated in
multiple places, triggering SonarCloud duplication warnings. Extract shared
constants for the state keys used by setOutputMimeType/getOutputMimeType and
setOutputQuality/getOutputQuality, and reuse those constants in the setState,
getPropertyRaw, and getState calls. Keep the existing behavior unchanged while
centralizing the literal names in ImageCrop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9659a7f1-801d-41a8-93f9-210f59c060f9
📒 Files selected for processing (7)
README.mdpom.xmlsrc/main/java/com/flowingcode/vaadin/addons/imagecrop/ImageCrop.javasrc/main/resources/META-INF/resources/frontend/src/image-crop.tsxsrc/test/java/com/flowingcode/vaadin/addons/imagecrop/ImageCropDemoView.javasrc/test/java/com/flowingcode/vaadin/addons/imagecrop/OutputFormatImageCropDemo.javasrc/test/java/com/flowingcode/vaadin/addons/imagecrop/test/ImageCropTest.java



Reworks the fix for #23 following the approach agreed in the PR #27 discussion: a combination of an explicit server-side output-format property and zero-network auto-detection. Supersedes #27.
Problem
The cropped image was always encoded as PNG (
canvas.toDataURL("image/png", ...)), so JPEGs (and any other format) were silently converted to PNG.What this does
setOutputMimeType(String)lets the caller choose the output format (image/png,image/jpeg,image/webp).data:URI, or the file extension of a regular URL.canvas.toDataURLcan actually encode (png/jpeg/webp); anything else falls back toimage/png.setOutputQuality(double)controls the encoding quality for lossy formats (jpeg/webp).Notes
Closes #23
Summary by CodeRabbit